[SPARK-3410] The priority of shutdownhook for ApplicationMaster should not be integer literal by sarutak · Pull Request #2283 · apache/spark

sarutak · 2014-09-05T05:35:04Z

I think, it need to keep the priority of shutdown hook for ApplicationMaster than the priority of shutdown hook for o.a.h.FileSystem depending on changing the priority for FileSystem.

…ook for ApplicationMaster higher than the priority of shutdown hook for HDFS

SparkQA · 2014-09-05T05:39:25Z

QA tests have started for PR 2283 at commit 717aba2.

This patch merges cleanly.

srowen · 2014-09-05T06:20:23Z

This change makes this shutdown hook lower than FileSystem's, whereas it used to be higher. Also does this compile for yarn-alpha too? Given the time it went in, it probably works with all supported Hadoop versions but worth checking.

sarutak · 2014-09-05T06:30:51Z

@srowen It's confused but lower value is higher priority.

srowen · 2014-09-05T06:33:15Z

Ah I see. That's fine, I just wasn't sure which the intent was since I think the original description is missing a word.

SparkQA · 2014-09-05T06:40:18Z

QA tests have finished for PR 2283 at commit 717aba2.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class AddJar(path: String) extends LeafNode with Command

sarutak · 2014-09-05T06:42:44Z

Ah, sorry it's my wrong. I confirm the logic of ShutdownHookManager, and higher value is higher priority.

sarutak · 2014-09-05T09:16:07Z

test this please.

sarutak · 2014-09-05T14:25:53Z

Jenkins, retest this please .

tgravescs · 2014-09-05T14:47:17Z

I don't think this is really necessary as I see the value of the Filesystem one as a public api now and changing its value would break compatibility, but I'm ok with it. Yes yarn-alpha has this defined.

Higher value is higher priority. I would rather leave it at value 30 or at least some priorities in between, so I would rather see + 20. 30 is also what mapreduce uses so if Hadoop would to add others in we would be better off to imitate MR.

tgravescs · 2014-09-12T14:17:43Z

@sarutak can you please either upmerge or close this

sarutak · 2014-09-12T14:37:36Z

Thank you for notification. I've rebased.

SparkQA · 2014-09-12T14:39:14Z

QA tests have started for PR 2283 at commit 2f0aee3.

This patch merges cleanly.

SparkQA · 2014-09-12T15:40:03Z

QA tests have finished for PR 2283 at commit 2f0aee3.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2014-09-12T19:13:43Z

thanks, can you please also address my concerns in the comment above.

sarutak · 2014-09-13T07:48:25Z

@tgravescs I agree with +20, and how about asserting the priority is between HDFS's and MR's?

SparkQA · 2014-09-13T08:19:21Z

QA tests have started for PR 2283 at commit 54eb68f.

This patch merges cleanly.

SparkQA · 2014-09-13T09:24:59Z

QA tests have finished for PR 2283 at commit 54eb68f.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2014-09-13T12:09:12Z

yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala

@sarutak an assertion is a good idea. Could you change the priority back to 30 instead of 20? Also instead of having 20 hardcoded in 2 different places can you make one static for this something like:

val SHUTDOWN_HOOK_PRIORITY: int = 30

in the ApplicationMaster object area.

sarutak · 2014-09-13T13:14:22Z

@tgravescs Oh, I mistook. The priority is duplicated.
Now I've modified. Thanks!

SparkQA · 2014-09-13T13:19:12Z

QA tests have started for PR 2283 at commit e8ce90b.

This patch merges cleanly.

SparkQA · 2014-09-13T13:24:12Z

QA tests have started for PR 2283 at commit ce86eb9.

This patch merges cleanly.

SparkQA · 2014-09-13T13:29:10Z

QA tests have started for PR 2283 at commit bd6cc53.

This patch merges cleanly.

SparkQA · 2014-09-13T14:25:56Z

QA tests have finished for PR 2283 at commit e8ce90b.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2014-09-13T14:30:17Z

QA tests have finished for PR 2283 at commit ce86eb9.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2014-09-13T14:35:27Z

QA tests have finished for PR 2283 at commit bd6cc53.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2014-09-14T02:14:17Z

QA tests have started for PR 2283 at commit 1d44fef.

This patch merges cleanly.

SparkQA · 2014-09-14T03:19:23Z

QA tests have finished for PR 2283 at commit 1d44fef.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2014-09-15T13:53:37Z

+1 looks good. Thanks @sarutak

Modified ApplicationMaster to make to keep the priority of shutdown h…

717aba2

…ook for ApplicationMaster higher than the priority of shutdown hook for HDFS

Removed unused import statements

217d1a4

Modified the priority for AM's shutdown hook

4c5cb93

sarutak changed the title ~~[SPARK-3410] The priority of shutdownhook for ApplicationMaster should not be integer literal, rather than refer constant.~~ [SPARK-3410] The priority of shutdownhook for ApplicationMaster should not be integer literal Sep 5, 2014

Merge branch 'master' of git://git.apache.org/spark into SPARK-3410

2f0aee3

Changed Shutdown hook priority to 20

54eb68f

tgravescs reviewed Sep 13, 2014
View reviewed changes

Added constant "SHUTDOWN_HOOK_PRIORITY" to ApplicationMaster

ee6f1aa

sarutak force-pushed the SPARK-3410 branch from e8ce90b to ce86eb9 Compare September 13, 2014 13:19

Modified style

bd6cc53

sarutak force-pushed the SPARK-3410 branch from ce86eb9 to bd6cc53 Compare September 13, 2014 13:22

Merge branch 'master' of git://git.apache.org/spark into SPARK-3410

1d44fef

asfgit closed this in cc14644 Sep 15, 2014

sarutak deleted the SPARK-3410 branch April 11, 2015 05:22

Conversation

sarutak commented Sep 5, 2014

Uh oh!

SparkQA commented Sep 5, 2014

Uh oh!

srowen commented Sep 5, 2014

Uh oh!

sarutak commented Sep 5, 2014

Uh oh!

srowen commented Sep 5, 2014

Uh oh!

SparkQA commented Sep 5, 2014

Uh oh!

sarutak commented Sep 5, 2014

Uh oh!

sarutak commented Sep 5, 2014

Uh oh!

sarutak commented Sep 5, 2014

Uh oh!

tgravescs commented Sep 5, 2014

Uh oh!

tgravescs commented Sep 12, 2014

Uh oh!

sarutak commented Sep 12, 2014

Uh oh!

SparkQA commented Sep 12, 2014

Uh oh!

SparkQA commented Sep 12, 2014

Uh oh!

tgravescs commented Sep 12, 2014

Uh oh!

sarutak commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

tgravescs Sep 13, 2014

Choose a reason for hiding this comment

Uh oh!

sarutak commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 13, 2014

Uh oh!

SparkQA commented Sep 14, 2014

Uh oh!

SparkQA commented Sep 14, 2014

Uh oh!

tgravescs commented Sep 15, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants